Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to configure timeout of Expect 100 behaviour #311

Closed
wants to merge 1 commit into from

Conversation

tailrecur
Copy link

curl 0.4.35 added the option to configure the timeout value that curl will wait for when using an Expect header before sending the request body.

This commit adds the ability to configure this timeout via the Configurable trait within isahc.

Fixes #303

@tailrecur tailrecur requested a review from sagebind as a code owner March 11, 2021 08:25
`curl 0.4.35` added the option to configure the timeout value that `curl` will wait for when using an `Expect` header before sending the request body.

This commit adds the ability to configure this timeout via the `Configurable` trait within `isahc`.

Fixes sagebind#303
@sagebind
Copy link
Owner

Thanks for working on this! Though I would still be interested in allowing the user to disable the Expect behavior entirely as well within the same option somehow. There's effectively two states for this feature:

  1. Expect is enabled, and has a specific timeout.
  2. Expect is disabled, and a timeout isn't applicable.

Generally I've been moving towards grouping options like this into independent structs because it makes it more clear when an option is applicable. For example, if enabling/disabling Expect were to be a separate option, it would be possible to do something like

Request::post("...")
    .expect_100_enabled(false)
    .expect_100_timeout(Duration::from_millis(100))

Which while a naive example, demonstrates how the user might get confused (and trust me, experience tells me that users will get confused about things that were obvious to me as an author of the code). If we can use types to completely prevent the user from setting a timeout if Expect is disabled, then the behavior is more self-documenting. If we add expect_100_timeout as-is now, we can't really change our mind about this.

Honestly, to be transparent I hate holding up PRs when it is sometimes hard enough to get people to contribute to open source as it is. But I also am inspired by the Rust project itself to hold a very high view of API design and want to make sure we think through all the angles before adding new APIs.

Anyway, here's some thoughts in my head right now:

  • expect_100 feels weird to say (probably because it isn't a noun but rather an imperative statement); is there a better word for describing this feature? If not we might just be stuck with it.
  • This feature is very enum-like (enum Expect100 { Disabled, Enabled(Duration) }) but I usually avoid enums in public APIs in favor of opaque structs if it is possible that more configuration points might be added to it in the future. (Since adding a field to an enum variant is a breaking change, but adding one to an opaque struct with a default value doesn't have to be breaking.)

sagebind added a commit that referenced this pull request Aug 22, 2021
Add a configuration option for changing how POST requests behave under HTTP/1.1 and the `Expect: 100-continue` request behavior. Allow the user to change the timeout to a different duration or to disable the header entirely.

Replaces #311.

Fixes #303.
sagebind added a commit that referenced this pull request Aug 22, 2021
Add a configuration option for changing how POST requests behave under HTTP/1.1 and the `Expect: 100-continue` request behavior. Allow the user to change the timeout to a different duration or to disable the header entirely.

Replaces #311.

Fixes #303.
@sagebind
Copy link
Owner

Closed in favor of #340.

@sagebind sagebind closed this Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration to disable Expect Header being set by default
2 participants